Independent review: Claude Opus 5.5 (xhigh), at head 4036702d. One of two parallel reviews (Codex Astra is the other); posted by the coordinator. Evidence paths named below are local to the review host.

PR #1030 review: Commerce contract and the IEggStock seam (Opus, independent)

• Head: 4036702d7526780b35bd506095edfc35d9133af7. Base: main d6dbd47d.
• Checkout: a detached worktree at /home/mforce/.cluckwork-slices/review-1030-opus, removed after the review.
• Method:
  • I read the confirm and void handlers line by line against main, and read the port, the planner move, the probe inversion, the peer ports, the endpoints, the seeders, the ledger, the matrix and the docs.
  • I formed my view before reading the PR body. There were no PR comments yet.
  • I ran targeted test classes and four mutations, and reverted each one.
  • CI on the head is all green.

Verdict on the core question

I found no product defect. Confirm and void behave the same as on main, step by step:

• Transaction boundary, save and audit. ExecuteInTransactionAsync is unchanged. There is still one SaveChanges, in UnitOfWork, after the lambda returns true. There is one audit write per path, inside the transaction.
• Lock statement. There is one FOR UPDATE over every grade, farm-wide, ORDER BY "ProductionDate", "Id" (EggStock.cs:11 → EggLotRepository.cs:43-60, SQL untouched). It runs at the same point: after the Account FOR SHARE, the order FOR UPDATE, the role re-read, the ceiling check and the assignment read.
• Two plans (Configurable Worker sales allocation scope by flock assignment #612). Both run over the same locked instances: Plan(demand, assignedFlockIds), then Plan(demand). There is no second query. The filter is the same Contains(l.FlockId).
• When Allocate runs. It runs only after a complete plan, on the same tracked instance, and a refusal throws InvalidOperationException with the same message as before. So the 500 and rollback behaviour is unchanged.
• Allocation created first. SalesOrderAllocation is now constructed before Allocate. It is untracked until AddRangeAsync, so the change-tracker insertion order and the SQL batch are unchanged.
• Void.
  • The payment gate, order.Void, the NoAllocationRecords refusal and the per-lot sum are unchanged.
  • RestoreAsync returns a Result, as the inline code did. The handler maps its error through unchanged.
  • Restore iterates the locked lots in canonical order and writes the Void movement with the same reference type, id and reason.
  • The MarkReleased provenance release follows, as before.
• Retry boundary (Add transient-DB resilience (EnableRetryOnFailure) on the request path #269). The port opens no transaction, creates no scope or execution strategy, and never calls SaveChanges. I checked EggStock, EggLotRepository and EggInventoryMovementRepository.
• [C] #514 slice 11: Egg Operations contract and the opaque stock seam #853's six seam constraints. One lock, two plans, apply only the chosen plan, Commerce mints the allocation id, void restores by lot, no save. All six hold.
• No EggLot in the contract surface. No contract type exposes EggLot. IEggStock, IEggStockReservation, SaleAllocationPlan, PlannedEggLotDraw and SaleDemandLine carry only ids, ints, dates, strings and Result.
• Currency probe.
  • The queries and their order are the same: SalesOrders, Payments, Expenses, priced Products, InventoryLots, FeedUsages, InventoryItems with a default cost.
  • It still short-circuits on the first yes, on the same scoped AppDbContext and so in the same transaction.
  • MS.DI resolves IEnumerable<T> in registration order, and ICurrencyBoundRowSource has no other registration.
  • Order cannot affect the answer. The answer is an OR over side-effect-free reads under the Close the §4.6 currency-lock race properly (shared lock on the account row across money-writing handlers) #162 Account lock. Order only changes how many queries run.
• Peer ports. IEggGradeLookup.GetAsync and IEggUnitConversionLookup.GetByUnitAsync forward to the same tracked FirstOrDefaultAsync, under the same tenant filter, and copy every field.

Findings

None of these findings is a product defect.

F1: ConfirmSaleAtomicityTests is red only when both rules break; a cheap change makes each rule fail alone

• Severity: P3 (test quality). Status: CONFIRMED.
• Location: tests/Cluckwork.Api.IntegrationTests/ConfirmSaleAtomicityTests.cs:82.
• Defect: the test checks only the persisted outcome, so it turns red only when the transaction AND the never-save rule both break.
• Failure scenario: a later change adds await unitOfWork.SaveChangesAsync(transactionCt) after each DrawAsync. Today that is harmless, because the transaction rolls it back. But the rule AGENTS.md now says to keep is no longer pinned. A second, unrelated change that removes the handler's transaction then breaks atomicity on the seeder path with no warning between the two changes. The same holds in reverse.
• Evidence:
  • Mutation B (PR's): a save after each draw → all 4 cases stay green. Reproduced.
  • I prototyped two checks at the fault point:
    • db.Database.CurrentTransaction is not null;
    • no tracked EggLot is Unchanged (that is, no draw was flushed early).
  • The prototype is a throwaway diff at /home/mforce/.cluckwork-slices/reviews/1030-4036702d-opus-atomicity-prototype.diff. It uses a static field; the real version should pass the context into the decorators.
  • Control (no mutation): 4/4 green.
  • Save after each draw alone: 4/4 red with a drawn lot was already saved before the fault.
  • No transaction alone (one save at the end): 4/4 red with the confirm runs outside a transaction.
• Recommendation: keep the test. It is a correct outcome test, and the PR is candid about what it can and cannot see. Add the two invariant checks, so each rule has its own red and "keep both" in AGENTS.md is enforced.
• Coverage note: the test runs outside HTTP, so it covers the owned-transaction path (seeders) only, not the path that joins IdempotencyMiddleware's transaction.

F2: "Commerce never sees EggLot" is stated as a rule, but nothing enforces it

• Severity: P3 (doc honesty). Status: CONFIRMED.
• Location: AGENTS.md:84 (the [C] #514 slice 12: Commerce contract, consuming the stock seam #854 sentence), module-ledger.json:227 (edge reason), docs/architecture.md.
• Defect: the AGENTS.md sentence reads as an enforced fact, but no test fails when Commerce reaches EggLot again.
• Failure scenario: a later change has ConfirmSaleHandler inject IEggLotRepository and plan with SaleAllocationPlanner.Plan(..., EggLot list) directly. Every architecture guard stays green, because the handler is already a declared symbol on the Commerce -> EggOperations edge and peers are not walked by type.
• Evidence:
  • Mutation D (mine, not in the PR's table): add IEggLotRepository eggLots to ConfirmSaleHandler, call GetAvailableFifoLockedAsync and pass the result to SaleAllocationPlanner.Plan.
  • Result: dotnet test tests/Cluckwork.Application.Tests --filter FullyQualifiedName~Architecture gave 338/338 green.
• What the docs already say: the 854 record says peers are unchecked, but its example is IEggUnitConversionRepository. [C] #514 slice 10: Flock Management contract, eligibility and mortality ports #852's AGENTS sentence carries the explicit caveat; [C] #514 slice 12: Commerce contract, consuming the stock seam #854's does not.
• Recommendation: append "Nothing checks this: a ConfirmSaleHandler that injects IEggLotRepository again stays green."

F3: Two ledger edge reasons still name IEggUnitConversionRepository

• Severity: P3 (ledger honesty). Status: CONFIRMED.
• Location: tests/Cluckwork.Application.Tests/Architecture/Data/module-ledger.json:167 (Access → Commerce) and :269 (Farm → Commerce).
• Defect: both reasons still say the handler "resolves ... through Commerce's IEggUnitConversionRepository". SetStepperUnitHandler and UpdateFarmSettingsHandler now take IEggUnitConversionLookup.
• Failure scenario: the Modular-monolith architecture design + implementation plan (from PR #423) #514 Track B and [C] #514 slice 15: Access contract — security hold point, runs last #857 slices plan from these reasons. They would look for a repository dependency that no longer exists, and miss that the lookup is now the seam.
• Evidence:
  • git diff d6dbd47d 4036702d -- .../SetStepperUnitHandler.cs .../UpdateFarmSettingsHandler.cs shows both switched to the lookup.
  • The two reason strings are unchanged in the ledger diff. The PR rewrote only the Commerce -> EggOperations reason.
  • No guard reads reason text, so CI cannot see this.

F4: IEggStockReservation.Plan is pure only until the first DrawAsync

• Severity: P3 (contract wording). Status: CONFIRMED by reading.
• Location: src/Cluckwork.Application/Features/EggLots/IEggStock.cs:23-25, docs/decisions/854-commerce-contract.md:56-57.
• Defect: the comment says Plan "plans the same way every time". In fact Plan seeds remainingByLot from each locked lot's live QuantityAvailable (SaleAllocationPlanner.cs:41), and DrawAsync mutates those same tracked instances.
• Failure scenario: a future caller plans, draws, then plans again, for example a partial-confirm retry or a second order in one reservation. The second plan silently differs from the first.
• Impact today: none. ConfirmSaleHandler never plans after drawing.
• Recommendation: either reword to "the same way every time until a DrawAsync", or snapshot quantities at lock time.

Nits

None of these is a defect. The draw-per-call shape is owner-approved and kept.

• DrawAsync accepts any draw (EggStock.cs:47). It does not check that the draw came from a complete plan of this reservation, or that it is applied only once. Drawing twice from a lot with enough stock double-allocates silently. The old inline code had the same freedom. Optional hardening: the reservation records applied draws and refuses a repeat.
• Nothing ties a reservation to its transaction. A reservation used after commit mutates tracked lots without the row lock, and only the Version token stops a lost write. The interface comment "Call it once per confirm" (IEggStock.cs:9) is also unenforced. Only one caller exists, and it holds the reservation in a local.
• Duplicate bodies. EggOperationsModule.GetGradeAsync (EggOperationsModule.cs:62) and EggGradeLookup.GetAsync now have identical bodies. One could delegate to the other.
• Cost of resolving CommerceModule (CommerceModule.cs:22). It constructs all 15 handlers, including IIdentityProvider's UserManager graph through ConfirmSaleHandler, on every Commerce read endpoint. On main, those endpoints resolved one repository. This is the same pattern as the earlier module contracts and affects adapters only.
• A currency source registers by hand (CluckworkFeatureServiceCollectionExtensions.cs:148-154). A repository that implements ICurrencyBoundRowSource but is not registered is skipped silently. Five of the seven sources have their own FarmSettingsTests refusal; Payments and FeedUsages cannot exist alone. The exposure is the same as the old hand-written OR chain. A test asserting "implementors == registrations" would close it.

Reproduced and verified

Not reproduced: the 54-file SQL trace. The script is not committed. My reading agrees with it: the lock SQL is untouched, the probe queries are verbatim, and no tracked entity changes order.

All mutations are reverted. The worktree was clean before removal.

Product defects: none. Findings: 4 × P3, all docs or tests.

Verdict: merge. F1–F4 are cheap follow-ups, and F3 and F2 are one-line text fixes that could ride along.

Review of record: Codex gpt-6-astra (xhigh), round 1, at head 4036702d. One of two parallel reviews (Opus is the other); posted by the coordinator. Evidence paths named below are local to the review host.

PR #1030 review by Astra

Reviewed 4036702d7526780b35bd506095edfc35d9133af7 against d6dbd47d. Review performed independently, without delegation or contact with the other reviewer.

One confirmed P2 contract defect. No product defect was demonstrated in the current routes or seeders.

1. P2, CONFIRMED. The stock reservation remains usable after its locking transaction ends.

   Location: src/Cluckwork.Application/Features/EggLots/EggStock.cs:47, with construction at lines 8–11 and retained state at lines 37–45.

   Reservation retains tracked EggLot instances and the movement repository, but captures no transaction identity or lifetime; neither Plan nor DrawAsync rejects an expired reservation.

   Concrete scenario: a non-HTTP caller retains the reservation returned inside ExecuteInTransactionAsync, lets that transaction commit, then plans and draws in the same service scope. DrawAsync succeeds after PostgreSQL has released the locks. A subsequent SaveChangesAsync persists the stock reduction and Sale movement independently of the transaction that was supposed to protect them. A later failure in the caller cannot roll that committed work back.

   Evidence: the throwaway real-Postgres test Scratch1030ReviewTests.ReservationAfterCommit_StillPlansAndPersistsAnUnlockedDraw verifies Database.CurrentTransaction == null, then successfully plans and draws ten eggs. A fresh context sees availability fall from 30 to 20 and one Sale movement. This diagnostic deliberately asserts that the forbidden usage succeeds. It passed on the unmodified head. Source and TRX evidence are retained beside this report.

   Product impact: not a demonstrated current product defect. ConfirmSaleHandler keeps its reservation local to the transaction, and I found no production caller retaining it. It does violate this review's explicit requirement that the reservation be unusable after its transaction. Scoped DI does not enforce that requirement: a scope can outlive a transaction. A disposed request's DbContext prevents later persistence through that context, but does not invalidate the reservation's in-memory planner.

   Required change: bind the reservation to its originating transaction and reject use after it ends, including use inside a different transaction. Keep persistence types out of the contract. Add a real test requiring rejection of the demonstrated escape.

The confirm/void comparison found no other behavioral change:

• Confirm preserves the account FOR SHARE, order FOR UPDATE, fresh role/ceiling/assignment checks, and one farm-wide lot lock over all grades in (ProductionDate, Id) order. Both planning passes use the same reservation. The planner copies available quantities and mutates only its local dictionary; assigned-flock selection retains the locked rows' order.
• The earlier allocation construction is safe for the actual planner output. Every draw quantity is positive because both remaining demand and available stock are positive before Math.Min. SalesOrderAllocation.Create only checks positivity, generates an ID and constructs an untracked object. AddRangeAsync still happens after all draws and order confirmation. An Allocate refusal still throws the same invariant exception before the final save; the extra local object cannot persist. Fresh SQL traces also matched.
• Void retains the payment gate under the order lock, groups pending allocations by lot, restores in canonical order, marks provenance released, and writes one audit row. Sale movements reference the allocation ID; Void movements reference the order ID and reason, exactly as [C] #514 slice 11: Egg Operations contract and the opaque stock seam #853 specifies. Void movements should not be changed to reference allocation IDs.
• Both handlers retain one business save through UnitOfWork. HTTP calls join the middleware's request transaction; direct calls own their transaction. AmbientTransaction and SingleAttemptExecution are unchanged. The seam neither saves nor starts a retry strategy. A separate diagnostic forced a contradictory second draw and confirmed the exception left the earlier draw and movement unpersisted.

Independent mutation results, each compiled and reverted:

The four-point atomicity test is meaningful as a check for partial persistence under those faults. It is not an independent guard for the explicit transaction or the no-save rule. All four injected faults occur before the normal final save, so the unmodified test does not exercise rollback of already-issued business writes. The combined mutation supplies those writes and proves the assertions detect them. The PR describes this limitation accurately; I do not report it as a separate defect. Detecting an added seam save depends on the reviewed code and the separately rerun SQL trace.

The currency probe preserves the seven predicates, their order and short-circuit behavior. A real DI resolution returned SalesOrder, Payment, Expense, Product, InventoryLot, FeedUsage and InventoryItem repositories in that order. These explicit registrations provide deterministic enumeration. The empty-farm trace runs all seven reads; the expense-farm trace stops after the third. Both deleteWhen #854 exceptions are gone.

The contract and ledger changes match the code: six Commerce implementations, eight new seeder exceptions assigned to #858, and the Commerce-to-EggOperations edge reduced from eight symbols to five because the planner types moved. No EggLot reaches the Commerce contract. I regenerated the coupling matrix; it remained byte-identical to the committed file. Endpoint response copies preserve the existing fields, including quantities, list-price basis, money denomination, versions where previously exposed, and discount reasons. The new decision record accurately describes the implemented flow and the disclosed guard limits.

I reran the PR's throwaway trace code on both commits in the assigned checkout. All four confirm/void scenarios matched byte-for-byte, as did the 24 route scenarios' SQL and normalized bodies. Across the complete output, 52 files were literally identical. The other two differed only in generated farm IDs in currency-probe response headers; their SQL and normalized headers matched. The recorder omits SQL parameter values and normalizes response IDs/timestamps, so this proves the recorded query shapes and normalized examples, not every possible input.

Verification: the final solution build passed with zero warnings/errors, including #985's style gate. All 132 selected existing integration cases, 29 selected Application cases, three additional diagnostic cases, and the three trace cases on each commit passed. Application guards passed again after mutation restoration. No full integration suite was run. I did not independently rerun the SPA suite or EF-model digest. The checkout is clean and restored to the reviewed head; no commits, pushes or GitHub comments were made.

Evidence: 1030-4036702d-astra-evidence/ beside this report contains diagnostic source, the mutation runner and logs, TRX results, and both sets of traces.

Nits: none.

Verdict: request changes for the reservation lifetime contract; no current product regression demonstrated.

Review round 1, fixed at bc3eb12e. One round for both reviews (Astra comment, Opus comment). origin/main (b88ec0eb, #1029, test-only) is merged in with a merge commit, 9c72c162. No force-push.

Behaviour is unchanged. I re-ran the SQL-trace script on bc3eb12e against main d6dbd47d; #1029 changed no source. All 54 files are byte-identical: confirm success, void success, short confirm and the payment-gated void (SQL and transaction events), both currency-probe scenarios, and 24 Commerce routes (SQL and normalised response bodies). The new transaction check reads Database.CurrentTransaction and issues no SQL.

Tests run locally on bc3eb12e:

• Application: 682/682. That is 681 plus PeerContractRealTreeTests; the matrix, ledger, contract and compatibility guards are included.
• Integration, targeted: 407/407. This covers the 384-test stock, sales, seed and currency set, plus EggStockTransactionTests, ConfirmSaleAtomicityTests, TransientDbResilienceTests and RetryBoundaryTests. SchemaDocsTests also passes.
• The build is clean with warnings as errors, which enforces the build: enforce file-scoped namespaces and using placement at build time #985 style gate. An IDE0005 build finds no unused usings in this round's files.

Not done, on purpose. I left Opus's nits as they are: duplicate grade-read bodies, DrawAsync not refusing a repeated draw, CommerceModule's construction cost, and no check that every source is registered. None was in scope for this round. The full integration suite runs in CI.

Review of record: Codex gpt-6-astra (xhigh), round 2, at head bc3eb12e. Posted by the coordinator. Evidence paths named below are local to the review host.

PR #1030, round 2, Astra review

Reviewed bc3eb12e9e4ca6b0e18bac8eb93d721df74673f6, scoped to the fixes since 4036702d. Reviewed independently, without delegation or contact with the other reviewer. Did not wait for CI.

The original P2 escape is fixed. One further P2 contract defect is confirmed below. No current product defect or legitimate-confirm regression was demonstrated.

1. P2, CONFIRMED. Comparing only the outer transaction ID accepts a reservation after savepoint rollback has released its row locks.

   Location: src/Cluckwork.Application/Features/EggLots/EggStock.cs:86; the identity comes from src/Cluckwork.Infrastructure/Persistence/CurrentTransaction.cs:7.

   Concrete scenario: begin a transaction, create savepoint S, call LockForSaleAsync, then roll back to S. PostgreSQL releases the locks acquired after S, but EF's current transaction and TransactionId remain unchanged. Both Plan and DrawAsync therefore pass EnsureLocked, even though the reservation's lots are no longer locked. The caller can save a draw using that expired lock protection.

   Evidence: the real-Postgres diagnostic Scratch1030Round2Tests.RollbackToSavepointBeforeLock_ReleasesLocksButReservationStillDraws checks all of these facts. A second connection's FOR UPDATE NOWAIT fails before the rollback and succeeds afterward; ICurrentTransaction.Id stays equal; planning and drawing still succeed; after commit, a fresh context sees availability reduced from 30 to 20 and one Sale movement. No production source mutation was needed. PostgreSQL explicitly documents that rollback releases locks acquired after the savepoint, including row locks. PostgreSQL locking documentation.

   Product impact: not a demonstrated current product defect. I found no explicit savepoint creation or rollback in src/. Confirm's normal EF savepoint occurs after its lot locks, so that normal sequence does not reproduce this escape. The defect is in the new lock-lifetime guarantee under the savepoint scenario requested for this review. The original post-commit escape is closed; this is a separate way to release the locks without changing the checked ID.

   Required change: account for rollback that releases the reservation's locks, and reject continued use of that reservation. This needs a lifecycle signal beyond the outer transaction ID, or an explicit, enforced restriction on savepoint recovery while reservations exist. Preserve the distinction between rollback past lock acquisition and savepoints created after the locks. Add the demonstrated case to the real transaction tests.

The original escape now fails for the intended reason. I reran the exact round-one diagnostic, which previously planned and persisted a draw after commit. It now throws InvalidOperationException from EnsureLocked at Plan, with This reservation's transaction has ended, so its lots are no longer locked. The new committed tests independently check direct DrawAsync rejection, a later transaction, and lock/restore with no transaction, including unchanged persisted stock.

The additional binding probes established:

AddDbContext<AppDbContext> and the repository, unit-of-work and ICurrentTransaction registrations are scoped. All those registered services receive the same context. CurrentTransaction.Id reads that context dynamically; it is not a captured constructor-time value or ambient task variable. No context split was found in the real composition.

The normal confirm transaction is not automatically replayed. SingleAttemptExecution captures failures inside the EF strategy and rethrows after it returns, preserving #269. My explicit replay probe intentionally used the lower-level retrying strategy to test a new transaction; it does not describe production confirm behavior. Freshly acquired reservations work on that new attempt. Both RetryBoundaryTests and TransientDbResilienceTests passed, including the bootstrap retry-boundary case selected by the filter.

The revised atomicity tests now catch each single regression. An added SaveChangesAsync inside DrawAsync, with the transaction intact, failed all four cases with a drawn lot was already saved before the fault. Removing only the confirm transaction failed all four at the port's new refusal. Disabling that refusal as well still failed all four at the test's own the confirm runs outside a transaction assertion. All mutations compiled and were reverted.

The peer guard works within its stated scope. Independent mutations adding IEggLotRepository to the confirm constructor, a nested IReadOnlyDictionary<Guid, IReadOnlyList<EggLot>> parameter, and GetService(typeof(IEggLotRepository)) each failed with the offending symbol and type. A control using contract types plus the intentionally retained Farm/Access dependencies passed. No false positive was observed. A method merely returning IEggLotRepository stayed green: the reused scanner reads parameters and service resolutions, not all type references. The decision record states that narrower scope; the guard should not be described as complete enforcement of every Commerce reference.

No persistence type crosses the actual ICurrentTransaction or IEggStock signatures. The real seam and module-contract guards passed. Adding an IQueryable<Guid> property to ICurrentTransaction made the seam guard fail with that exact interface/member, then was reverted.

Confirm and void handler bodies are unchanged by this round. The new transaction checks issue no SQL. I reran the three trace cases on this head and compared all 54 outputs against my independently recorded d6dbd47d baseline from round one. All recorded SQL, transaction events and normalized response bodies matched. As before, the two currency-probe headers require normalization of their generated farm IDs; SQL parameter values are not recorded by this script. The ledger reason and Plan comment corrections match the code.

Verification: 51 targeted integration cases passed, including transaction, atomicity, lock-order, allocation-policy, confirm/void race and retry suites. Ten targeted Application guards passed. Five additional binding diagnostics passed, one deliberately demonstrating the savepoint defect. The old escape diagnostic failed as expected at the new refusal; the other two round-one diagnostics and all three trace cases passed. After restoring mutations, the ten Application guards and seven transaction/atomicity cases passed again. The final solution build had zero warnings and errors. No full integration suite was run.

Evidence: 1030-bc3eb12e-astra-evidence/ beside this report contains diagnostic source, mutation scripts/logs, TRX results and the new traces. The assigned checkout is clean at bc3eb12e; all temporary test files and mutations were removed. No commits, pushes or GitHub comments were made.

Nits: none.

Verdict: request changes for savepoint invalidation; the original escape is fixed and no current product regression was demonstrated.

Review round 2, documented at ae905442. This replies to Astra's round 2 (comment). The commit changes documentation and comments only; no code changes.

The savepoint edge is real. Rolling back to a savepoint taken before LockForSaleAsync releases the reservation's row locks, but the transaction id stays the same, so the reservation would still plan and draw. The owner chose to record this limit rather than track savepoints:

• IEggStockReservation's comment (IEggStock.cs) now says the check compares transaction ids only. A reservation must not be used after a rollback to a savepoint taken before it was created, because PostgreSQL releases its locks while the id stays the same.
• docs/decisions/854-commerce-contract.md says the same and records that the owner chose to document it (PR refactor(sales): put Commerce behind a contract and reach egg stock through IEggStock #1030, review round 2).

Both say that nothing in src/ creates or rolls back to such a savepoint. grep -rn "Savepoint\|SAVEPOINT" src --include=*.cs finds nothing. The only savepoint is the one EF takes inside SaveChanges, after the locks, as the SQL trace's TX SAVEPOINT line shows.

Checks on ae905442: the solution builds, and the Application documentation guards (20) and SchemaDocsTests (4) pass.

The review loop was stopped deliberately by the owner after two consecutive rounds that found no product defect (rounds 1 and 2 found only seam-contract and test gaps). No round 3 will be triggered.

Independent review: Claude Fable 5.1 (xhigh), at head ae905442. Third reviewer, requested by the owner; posted by the coordinator. Evidence paths named below are local to the review host.

PR #1030 review: Commerce contract and the IEggStock seam (Fable, independent third review)

• Head: ae905442cf1b7d5c441199fad98db1bdbea76cb2. Base: main b88ec0eb.
• Checkout: detached worktree /home/mforce/.cluckwork-slices/review-1030-fable, removed after the review.
• Method. I read the full diff (49 files) and compared the confirm and void handlers line by line against main before reading the PR body, the two Codex Astra rounds and the Opus round. I then built the Application and Integration test projects, ran the new and changed test classes on the clean head, and ran four mutations none of the earlier rounds tried. Every mutation was reverted; git status was clean before removal. I did not run the full integration suite.
• CI on the head: every check green (domain, application, apphost, integration, build, web, both image legs, CodeQL, GitGuardian).

Verdict on the core question

No product defect. Confirm and void do the same work, in the same order, against the same rows, inside the same transaction, as on main.

• Transaction boundary. Both handlers still run one ExecuteInTransactionAsync. UnitOfWork saves once after the lambda returns true. HTTP calls join IdempotencyMiddleware's transaction; direct calls (seeders, tests) own one through SingleAttemptExecution. The port opens no transaction and never saves (EggStock.cs: no SaveChanges, no BeginTransaction, no execution strategy).
• Lock count, order and scope. One FOR UPDATE per confirm, farm-wide, every grade on the order, ORDER BY "ProductionDate", "Id". The SQL never left EggLotRepository.cs:35-60. LockForSaleAsync is called once (ConfirmSaleHandler.cs:217), after the Account FOR SHARE, the order FOR UPDATE, the fresh role read, the ceiling check and the assignment read, exactly where GetAvailableFifoLockedAsync was called before.
• Two Configurable Worker sales allocation scope by flock assignment #612 passes over the same locked rows. stock.Plan(demand, assignedFlockIds) then stock.Plan(demand) (ConfirmSaleHandler.cs:229,238). Both run SaleAllocationPlanner.Plan over the one locked list; the assigned filter is the same Contains(l.FlockId) the handler used inline. No second query.
• Allocate and Restore. DrawAsync calls lot.Allocate(draw.Quantity, saleDate) on the same tracked instance the plan read, with the allocationDate the handler passed to the lock; a refusal throws the same InvalidOperationException text. RestoreAsync iterates the locked lots in canonical order and calls lot.Restore as the inline loop did.
• The allocation object created before Allocate. SalesOrderAllocation.Create is pure (positivity check, Guid.NewGuid(), no clock, no tracking). Both objects stay in memory until allocations.AddRangeAsync after the loop, which is where they were added before. Change-tracker insertion order is unchanged: movements added per draw, allocations after the loop.
• Movement rows. Sale: -quantity, reference type SalesOrderAllocation, the allocation id. Void: +quantity, reference type SalesOrder, the order id, the reason. Same arguments to EggInventoryMovement.Create as on main.
• Payment gate, Version, 409s, provenance release. Untouched lines in VoidSaleHandler; the only edit replaces the inline lock-and-restore loop with eggStock.RestoreAsync (VoidSaleHandler.cs:89) and maps its Result error through unchanged.
• Retry boundary (Add transient-DB resilience (EnableRetryOnFailure) on the request path #269). ICurrentTransaction cannot refuse a legitimate confirm. SingleAttemptExecution never replays the lambda, EF suspends nested retries inside the strategy, and the joined path uses the middleware's single transaction object, whose TransactionId is fixed at BeginTransactionAsync. The only replaying strategy in src/ is DemoDataSeeder's cleanup block, which runs no confirm. A reservation is a local in the lambda, so even an outer replay would mint a fresh one.
• Failure paths. Insufficient stock on any line is decided by the whole-order plan before any draw. An exception mid-draw propagates out of the lambda, and the owned scope disposes the transaction without commit; the joined path's middleware rolls back and clears the tracker, as before. Two confirms queue on the lot locks and the loser re-plans over the committed rows. A void racing a confirm queues on the same canonical order. A cancelled token mid-draw is an exception like any other. Status codes are unchanged because every refusal Error code and description string is byte-identical.
• Data. No persisted row changes field, order or timestamp. The PR's SQL trace claim is consistent with the reading; I did not rerun the script.
• DI. ICurrentTransaction, IEggStock, both repositories and IUnitOfWork are scoped on the one AddDbContext<AppDbContext> (CluckworkFeatureServiceCollectionExtensions.cs:106,382). The only hand-built contexts in src/ are ExportQueries' read-only snapshot and the design-time factory; neither feeds the port. IEnumerable<ICurrencyBoundRowSource> resolves in registration order, which is the old probe's order; the answer is an OR over side-effect-free reads, so order can only change the query count.
• Currency probe. Seven predicates verbatim, same order, same short-circuit, same scoped context.

Findings

Neither finding is a product defect. Severity is P3 for both.

F1: the sale-to-line provenance id is unpinned by any test (test coverage, CONFIRMED)

• Location. src/Cluckwork.Application/Features/EggLots/SaleAllocationPlanner.cs:52 (PlannedEggLotDraw(item.LineId, …)), consumed at ConfirmSaleHandler.cs:292 (SalesOrderAllocation.Create(…, draw.LineId, …)).
• Defect. This PR renamed the draw's SalesOrderItemId to LineId and moved the planner across a module boundary, and no test anywhere asserts which line an allocation row belongs to. SalesOrderAllocation.SalesOrderItemId is product data: the Admin CSV export writes it (ExportQueries.cs:193-196, column salesOrderItemId), and it is the sale-to-lot traceability chain spec §9.6 names.
• Failure scenario. A later edit stamps every draw with the first line's id. The FK to SalesOrderItems still holds, confirm, void and FIFO still work, the export carries wrong line ids for every multi-line order, and nothing goes red.
• Evidence. Mutation A: item.LineId → items[0].LineId in the planner. Built clean. SaleAllocationPlannerTests, CommerceModuleTests, EggOperationsModuleTests: 21/21 green. Integration VoidSaleTests, SaleAllocationPolicyTests, SalesOrderAuditPayloadTests, EggLedgerTests, every *Export* class and ConfirmSaleAtomicityTests: 74/74 green. grep -rn SalesOrderItemId tests finds no assertion on an allocation row; SaleAllocationPlannerTests.RepeatedGrade_AcrossTwoLines_SharesTheSameLotWithoutDoubleCounting (line 41) checks quantities only.
• Recommendation. Add Assert.Equal(order[0].LineId, plan.Draws[0].LineId) and order[1].LineId for the second draw in that planner test. It is a two-line change and makes the rename this PR performed a guarded one.

F2: AGENTS.md says the peer guard "reads signatures"; it reads parameter types and service resolutions (doc wording, PLAUSIBLE)

• Location. AGENTS.md:84, the [C] #514 slice 12: Commerce contract, consuming the stock seam #854 sentence "though it reads signatures, not method bodies". The decision record states the narrower truth at docs/decisions/854-commerce-contract.md:124.
• Defect. A return type is part of a signature, and AdapterReachScanner.AdapterTypes (AdapterReachScanner.cs:351-361) collects ParameterSyntax types and resolver-call type arguments only. A Commerce method returning IEggLotRepository or EggLot stays green.
• Evidence. Reading of the scanner; Astra's round-2 probe (a method returning IEggLotRepository stayed green) agrees. Not reproduced by me, hence PLAUSIBLE.
• Recommendation. Replace "signatures" with "parameter types and service resolutions" in AGENTS.md so the two documents agree, per the rule that successive corrections keep drifting when copies differ.

Already raised in earlier rounds, not re-raised here

• Opus's coverage note that ConfirmSaleAtomicityTests exercises the owned-transaction path only. True; the joined path is covered indirectly by the HTTP race tests.
• The savepoint limit (owner-accepted, documented). Nothing in src/ creates one before the lock.
• DrawAsync accepting any draw and CommerceModule's construction cost (Opus nits, owner left as is).

Nits

• EggOperationsModule.GetGradeAsync and EggGradeLookup.GetAsync have identical bodies. One could delegate to the other. Already noted by Opus.
• CommerceModuleTests pins that an unmapped product reports Guid.Empty, not null, for EggGradeId (line 91-101). That is faithful to main's Dictionary<Guid,Guid>.GetValueOrDefault, so the PR is right to keep it, but the pinned value is an accident of the old code rather than a design. Worth a follow-up, not a change here.
• services.AddScoped<ICurrencyBoundRowSource, SalesOrderRepository>() and the ISalesOrderRepository registration yield two repository instances per scope. Harmless because the repositories are stateless over the shared context.

Reproduced and verified

All four mutations were reverted; git status --short printed nothing before the worktree was removed.

Not reproduced: the 54-file SQL trace (script not committed; Astra re-ran it twice), the two-grade lock mutation (run by the PR, Opus and Astra, all red), the EF model digest.

Summary

• Product defects: none. The seam is a faithful extraction; every behaviour difference I looked for reduced to the same SQL, the same tracked instances and the same change-tracker order.
• Findings: 2 × P3. F1 is a real coverage hole exposed by this PR's own rename (CONFIRMED by a 95-test-green mutation); F2 is a one-word doc drift.
• Earlier reviews. I agree with Opus and both Astra rounds on the handler equivalence. The reservation-lifetime fixes hold under a weaker-check mutation they did not run.

Verdict: merge. F1 is a two-line test addition that could ride along or follow.

Fable review, both items fixed at 04222ba6. No new review round.

• F1 (allocation rows not pinned to their line). New SaleAllocationLineTests confirms a two-line order (Large 10, Medium 4) and requires each allocation row to name the line whose grade it drew. Your mutation, the planner stamping every draw with items[0].LineId, fails it 3 of 3. So does the handler-side version, SalesOrderAllocation.Create(…, order.Items[0].Id, …), 3 of 3. In both, the 4-egg Medium draw names the Large line. Both mutations reverted. I tested persisted rows rather than the planner, so the handler's mapping is pinned too.
• F2 (guard wording). AGENTS.md and the decision record now both say PeerContractRealTreeTests reads only parameter types and service resolutions, not return types or method bodies. I checked the limit: a ConfirmSaleHandler method returning IEggLotRepository keeps it green.

Checks: the solution builds clean. The new test passes, as do the documentation guards (20) and SchemaDocsTests (4). No production code changed in this commit.